Skip to content

Conversation

@VeckoTheGecko
Copy link
Contributor

@VeckoTheGecko VeckoTheGecko commented Apr 28, 2025

This PR cleans up the state on the Field object (building on #1946)

This PR implements the following changes:

  • remove _location
  • remove _vertical_location
  • remove _spatialhash
  • remove _gtype
  • remove _lonlat_minmax
  • add gridadapter
    • provides the Field with access to the grid adapter where its needed. eventually we'll get rid of this class entirely
  • update GridAdapter to use composition
  • add _core.utils subpackage to put helper functions etc
  • update Field init to use overloads
For context, here is the discussion I had with @fluidnumerics-joe

Needed state on Field

Field:

  • name -> needed
  • data -> needed
  • grid -> needed
  • _interp_method -> needed
  • igrid -> needed
  • _parent_mesh -> Remove. gives indication of the model its coming from. Unstructured specific? Can be determined from the attrs dict.
  • _location -> Tells us if face, node, edge registered (uxarray). Remove in favor of data.attrs['location']
  • _vertical_location -> remove in favor of data.attrs....
  • _mesh_type -> can be removed down the line
  • units -> Keep for now. Can likely be removed down the line. Nick thinks that this can just be removed from the codebase to a large extent where _mesh_type is picked up from attrs (based on units of lon/lat), and units for the variables are picked up. If units are degrees, a dask operation can be done to convert (Nick to investigate where unit converters are usd in the codebase).
  • allow_time_extrapolation -> needed? Let's remove entirely? Joe posted issue
    • if its a single timeslice, then we can extrapolate the time
    • otherwise, just use the provided time domain. Users (if they want extrapolation, or periodic looping of their simulation) can use xarray methods to create their dataset of interest (xr.concat etc.). Q:Does xr.concat actually increase the size of the dataset if you concat multiple of the same dataset? Or does it just create pointers to the same location?
  • _spatialhash -> get rid of (opt fordata.uxgrid.spatial_hash is None etc.... and calculate on the fly)
  • _gtype -> via GridAdapter (if need be)
  • _lonlat_minmax -> via GridAdapter (if need be)

  • Chose the correct base branch (field-testing which will go into v4-dev)
  • Fixes None
  • Added tests

Copy link
Contributor Author

@VeckoTheGecko VeckoTheGecko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One thing I'm a bit concerned about is managing state that is specific to the (un)structured Fields and their implementations within Parcels. It would be good to keep Field "grid agnostic", meaning grid specific logic is managed within the grid object1. The problem with this is that, for unstructured grids, we can't just add data to uxarray.Grid since we don't have control over that class, so we might need to go for some other mechanism2.

@fluidnumerics-joe is there any state that you think you'll need to save for unstructured grids in Parcels (i.e., that doesn't make sense to put in uxarray upstream?). I haven't changed the n_face property of Field yet - would be great to know your thoughts on that.

Footnotes

  1. This helps with abstraction: making the code easier to read and no need for a lot of "if" statements to define variables in one case for unstructured or another for unstructured

  2. This could either be caching (e.g., lrucache) for items not specific to a field object, or by adding an object unstructured_state to the Field class 3

  3. I love footnotes

Base automatically changed from field-testing to v4-dev April 29, 2025 12:22
get_spatial_hash() returns the cached spatial hash be default anyway
The adapter now wraps a Grid instance instead of inheriting from it. This provides better separation of concerns and makes the adapter's purpose more explicit - it's adapting an existing grid rather than extending it. It also allows for more flexibility in adapting any grid instance, regardless of how it was constructed.
@VeckoTheGecko VeckoTheGecko merged commit 8f56d6e into v4-dev Apr 29, 2025
2 of 8 checks passed
@VeckoTheGecko VeckoTheGecko deleted the field-init-cleanup branch April 29, 2025 12:53
@github-project-automation github-project-automation bot moved this from Backlog to Done in Parcels development Apr 29, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants